Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cpu/esp: change dynamic SSID option handling #17415

Merged

Conversation

gschorcht
Copy link
Contributor

Contribution description

This PR simplifies the definition of the option to use dynamic SSIDs for the SoftAP, especially with regard to the migration to Kconfig.

The semantics of defining a separate SSID prefix string that overrides the already defined SSID and enables the dynamic SSID generation with that prefix exactly when and only when it is set, made the handling of the parameter definition unnecessarily difficult and hard to understand.

Defining a boolean option that just enables the dynamic SSID generation, which then simply reuses the defined SSID as a prefix, makes it much more understandable and easier to handle, especially with respect to Kconfig.

This PR removes the configuration parameter ESP_WIFI_PREFIX and introduces the boolean configuration parameter ESP_WIFI_SSID_DYNAMIC which is defined as 0 by default. If set to 1, the already defined ESP_WIFI_SSID is used as prefix to generate a dynamic SSID by extending it with the MAC address of the SoftAP interface used, e.g.: RIOT_AP_aabbccddeeff

Testing procedure

Compile and flash test application

USEMODULE=esp_wifi_ap \
CFLAGS='-DESP_WIFI_SSID=\"MySSID\" -DESP_WIFI_PASS=\"MyPassphrase\" -DESP_WIFI_MAX_CONN=1'  \
make -C examples/gnrc_networking BOARD=esp32-wroom-32 flash

with and without -DESP_WIFI_SSID_DYNAMIC=1 in CFLAGS and use any WiFi device to check the used SSID.

Without -DESP_WIFI_SSID_DYNAMIC=1, the SSID of the SoftAP should be RIOT_AP, with -DESP_WIFI_SSID_DYNAMIC=1, the SSID of the SoftAP should be something like RIOT_AP_<mac>

Issues/PRs references

The semantics of defining an SSID prefix that overrides the already defined SSID exactly when and only when it is set, and then enabling dynamic SSID generation with that prefix, made handling the parameter definition unnecessarily difficult and hard to understand.

Defining a boolean option that enables dynamic SSID generation, which then simply reuses the defined SSID as a prefix, makes it much more understandable and easier to handle, especially with respect to Kconfig.
@github-actions github-actions bot added Area: cpu Area: CPU/MCU ports Area: doc Area: Documentation Platform: ESP Platform: This PR/issue effects ESP-based platforms labels Dec 16, 2021
@gschorcht gschorcht added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: cpu Area: CPU/MCU ports Platform: ESP Platform: This PR/issue effects ESP-based platforms and removed Area: doc Area: Documentation Platform: ESP Platform: This PR/issue effects ESP-based platforms Area: cpu Area: CPU/MCU ports labels Dec 16, 2021
Copy link
Contributor

@jeandudey jeandudey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK.

@gschorcht
Copy link
Contributor Author

@jeandudey Can we merge it?

@jeandudey jeandudey merged commit 466fdf5 into RIOT-OS:master Dec 17, 2021
@jeandudey
Copy link
Contributor

Yes, tested on esp32-wroom-32 on both configs

@gschorcht gschorcht deleted the cpu/esp/wifi_ap_dynamic_ssid_option branch December 18, 2021 09:42
gschorcht added a commit to gschorcht/RIOT-Xtensa-ESP that referenced this pull request Dec 19, 2021
Previously, a default value for ESP_WIFI_PASS was intentionally defined only if DOXYGEN was also defined, to allow ESP_WIFI_PASS to be left undefined for using APs without authentication. With PR RIOT-OS#17415 the definition was changed to always define a default value for EPS_WIFI_PASS.  This made it impossible to use APs without authentication. The commit reverts this change.
/**
* @brief Passphrase used for the AP as clear text (max. 64 chars).
*/
#ifdef DOXYGEN
#ifndef ESP_WIFI_PASS
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ESP_WIFI_PASS was intentionally defined only when DOXYGEN is also defined to allow ESP_WIFI_PASS to be left undefined for using APs without authentication. There are no other changes in this PR affected by this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ESP Platform: This PR/issue effects ESP-based platforms Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants